Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix server installer #68

Merged
merged 6 commits into from
Aug 3, 2023

Conversation

malte0811
Copy link
Contributor

  • net.neoforged.fancymodloader:loader is already an installer dependency, so adding it explicitly causes the "duplicate key" error (first commit).
  • Once that is fixed, you get errors along the lines of Missing language javafml version [24,] wanted by forge-1.20.1-47.1.57-universal.jar. This can be fixed by adding the corresponding JARs to the legacyClasspath by making them installer dependencies (second commit); I do not understand the NF launch process well enough to say whether this is the "right" fix or just a workaround though. On the client all of these jars are passed in the "real" classpath (-cp), which I assume has a similar effect.
  • This results in an error Module fml_language_java reads more than one module named fml_core, which can be fixed by adding pluginLayerLibrary and gameLayerLibrary dependencies to the ignoreList property (third commit). Again I am not sure whether this is the right solution, but it mirrors what is done in the client installations.

closes #38

@CLAassistant
Copy link

CLAassistant commented Jul 30, 2023

CLA assistant check
All committers have signed the CLA.

@Matyrobbrt Matyrobbrt added bug A bug or error 1.20 Targeted at Minecraft 1.20 triage Needs triaging and confirmation labels Jul 30, 2023
@sciwhiz12 sciwhiz12 removed the triage Needs triaging and confirmation label Aug 2, 2023
@sciwhiz12
Copy link
Member

While this PR gets most of the way there to getting the dedicated server running, an issue is still present which blocks the server from running on Windows: the FancyModLoader language providers aren't getting discovered by FML, leading to a crash as the minecraft system mod (provided by the FML Minecraft language provider) is missing.

In brief, I've determined the root cause to be the use of File.separatorChar in FML's FileUtils#matchFileName. File.separatorChar's value depends on the running operating system -- per its javadoc, it is / (forward slash) on UNIX systems, and \ (backslash) on Microsoft Windows systems.

However, the legacy classpath is always written using / forward slashes in Util#getArtifacts. This means that the dedicated server is able to match the file names of the language providers (passed as the fml.pluginLayerLibraries system property) against the classpath only on UNIX systems as File.separatorChar is / there.

I'm currently in the process of testing a fix in FancyModLoader for this. The PR author has consented to me or another maintainer updating this PR with the new FML version once that has been tested, merged, and published.

Fixes an issue where the language providers couldn't be discovered
properly, causing a crash.
@sciwhiz12
Copy link
Member

I've updated this PR with the version bump for FancyModLoader, as said above. I've tested the change locally, and it works -- the dedicated server now boots up fully.

@cpw
Copy link
Member

cpw commented Aug 2, 2023

This looks like the wrong solution to the problem. If I understand correctly, you basically fixed it by forcing all the modules into the legacy classpath instead. a COMPLETELY incorrect solution.

@malte0811
Copy link
Contributor Author

It's the same solution the other launch setups I looked at (userdev server+client, prod client) use, so I assumed that it was the correct one. If this PR is not usable please close it, I will not be able to work on this for the next 1.5 weeks.

@cpw
Copy link
Member

cpw commented Aug 2, 2023

Please do not merge this in the current state or anything like it. this looks completely wrong. I will be investigating tomorrow.

@cpw cpw self-requested a review August 2, 2023 18:27
Copy link
Member

@cpw cpw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the solution. There's a bigger picture issue.

@sciwhiz12 sciwhiz12 marked this pull request as draft August 2, 2023 18:29
@SizableShrimp
Copy link
Contributor

I can say at the very least that duplicating the artifacts from the pluginLayerLibrary and gameLayerLibrary configurations to the installer configuration manually is not the way to go. These deps are already installed yet not properly added to the clas spath in thewin_args.txt / unix_args.txt files that the server uses at startup. (This is a mistake from me when I implemented the feature initially at 5am 3 weeks ago.) The team will be looking into this with a close eye and our own fixes/cleanup in the next coming days. Thanks.

…se that breaks client. Should be merging the LayerLibraries into the CLASS_PATH for discovery by BSL
cpw
cpw previously approved these changes Aug 3, 2023
@cpw
Copy link
Member

cpw commented Aug 3, 2023

OK. The fundamentals were correct - i was misinterpreting the classpath discussion. However, you can't futz with the installer configuration, because that breaks the client. You have to fix the server install to merge the LayerLibrary files into the CLASS_PATH, as well as fixing the ignore.

@sciwhiz12 sciwhiz12 marked this pull request as ready for review August 3, 2023 18:00
@SizableShrimp SizableShrimp merged commit 809df1d into neoforged:1.20.x Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Targeted at Minecraft 1.20 bug A bug or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server Failure to Start - Duplicate key
6 participants